video: st_mipid02: addition of ST MIPID02 CSI bridge #89764

avolmat-st · 2025-05-10T14:59:13Z

The ST-MIPID02 is a CSI deserializer (up to 2 lanes) allowing to output up to 12 bit parallel interface.
It is present on the STM32MP135F discovery board and is part of the camera pipeline for this board:

GC2145 (sensor CSI) -> ST-MIPID02 -> DCMIPP (parallel interface)

This PR includes the driver and test entry for build_all. It will be added within the stm32mp135f-dk once the dcmipp PR #89407 will be merged.

This PR depends on the CCI interface PR #87935 as well as the GC2145-CSI PR #89571 in order to have the VIDEO_MIPI_ macros.

josuah

I could not spot blocking issues with it.
Interesting chip for Zephyr which has plenty of DVP-only boards...
Thank you for this driver!

drivers/video/video_st_mipid02.c

josuah · 2025-05-10T16:39:44Z

drivers/video/video_st_mipid02.c

+	link_freq = ctrl.val64 * video_bits_per_pixel(desc->csi_pixelformat) /
+		    (2 * cfg->csi.nb_lanes);
+
+	/* Enable lanes */
+	ret = video_write_cci_reg(&cfg->i2c, MIPID02_CLK_LANE_REG1,
+				  (2000000000 / link_freq) << MIPID02_CLK_LANE_REG1_UI_SHIFT |
+				  MIPID02_CLK_LANE_REG1_EN);
+	if (ret < 0) {
+		return ret;
+	}


I was just curious if link_freq was including some margin for vertical/horizontal blanking margins and rounding in this calculation.

No it doesn't here. Actually often the link_freq of the sensor is known, so in this case it is better to directly get the LINK_FREQ value from the sensor driver. When it isn't known it is calculated from the pixel_rate. This involve that the pixel_rate returned by the sensor is roughly right in order to get the correct link_freq.
Probably I could add first the LINK_FREQ control on the sensor side so that the if this one is available then we directly use it. There could be a helper for that to first try to pick the link_freq and in fallback if not available we use the pixel_rate to approximate it. Similar thing is done on Linux in fact. For the time being we don't have yet much CSI receiver in Zephyr so if we want to add stuff like that this might be a good time.

Thank you, I did not know how it was all stitched together. Now I do a little better!

if we want to add stuff like that this might be a good time

Putting it in a separate PR would make this one faster to merge, but also one more PR to wait for. Both work!

Hum, there are already several PRs which tend to get inter-dependent so I'll properly keep it like that for the moment to let those PRs get merged and will make a dedicated PR for such video-common general stuff, update required driver as well at same time.

Probably I could add first the LINK_FREQ control on the sensor side so that the if this one is available then we directly use it. There could be a helper for that to first try to pick the link_freq and in fallback if not available we use the pixel_rate to approximate it. Similar thing is done on Linux in fact. For the time being we don't have yet much CSI receiver in Zephyr so if we want to add stuff like that this might be a good time.

I think this is a good occasion to add the link frequency control which is better than pass by the pixel rate, no ? I didn't add it when adding pixel rate as there is no user of it at that time.

Hum, there are already several PRs which tend to get inter-dependent so I'll properly keep it like that for the moment to let those PRs get merged and will make a dedicated PR for such video-common general stuff, update required driver as well at same time.

I think we can put it here in the same PR (as it is related stuff) or in a separate PR (to get merged faster), it doesn't matter. Inter-dependency is common and not a problem as long as we add the dependency tree in the PR description.

tests/drivers/build_all/video/app.overlay

avolmat-st · 2025-05-12T12:56:56Z

I'm adding a new function video_get_link_frequency within the video-common in order to get the link-frequency from the source. If the link-frequency ctrl is not available (which is the case for the time being), it falls back on the pixel-rate.

avolmat-st · 2025-05-12T13:58:49Z

I've added the VIDEO_CID_LINK_FREQUENCY ctrl and a helper function. Moreover I've moved the * @name MIPI CSI2 Data-types enum in this PR so that it doesn't have dependency on the GC2145 one. I'll update the GC2145 PR in order expose the LINK_FREQUENCY ctrl instead of the PIXEL_RATE which is not really necessary now.

avolmat-st · 2025-05-12T20:15:33Z

New version pushed with PR #87935 commit added first in order to allow proper build of the PR.

ngphibang · 2025-05-13T07:47:11Z

drivers/video/video_ctrls.c

+	case VIDEO_CID_LINK_FREQUENCY:
 		*type = VIDEO_CTRL_TYPE_INTEGER64;
 		*flags |= VIDEO_CTRL_FLAG_READ_ONLY;
 		break;


I think that link frequency control is rather an array of values (integer menu), no ? This is one of the main differences and adavantages between it and the pixel rate. And we can specify the supported link frequencies in the DT like:

&camera { port { camera_ep: endpoint { clock-lanes = <0>; data-lanes = <1 2>; link-frequencies = /bits/ 64 <400000000 800000000>; // In Hz }; };

};

It's because the link frequency is board-specific characteristic (not only determined by the sensor) and for some sensors, the master clock is provided by the upstream device.

This way, sensor driver will get the link frequency from the DT property and create the menu.

But if it seems complicated and not needed for the current usecase, we can do it later.

Indeed. thanks for pointing it out. I mistaked the type it should be an interger menu. From the ST-MIPID02 point of view, it doesn't change much, it will read the value.
The ctrl has to be an integer menu and I will change that. Changes are actually more on the sensor (or source) side. To me the parsing of the DT and kind of automatic filtering of sensor capabilities / supported formats vs board allowed link-frequencies is a new helper that would have to be put in place. If you agree with that I'd like to postponed that to another PR.
I will post a new version with the fix of the ctrl type and on the GC2145 side I'll update that as well to use that as an integer menu. Then in a further PR we can introduce the helper to filter out based on link-frequencies properties from the DT.

I think it's fine like that, as long as the ctrl type is consistent, other things can be added and improved later. Thanks

avolmat-st · 2025-05-13T17:47:05Z

New version, with LINK_FREQUENCY now being a MENU_INTEGER type. Actually this type was not available yet, only MENU (strings) were available hence I've added a new intermediate commit to add this one prior to actually adding the new control itself. Now the helper is relying on this new type, queryctrl etc to figure out the link frequency value.

I've also update my gc2145 driver to work like that. If this one (ST-MIPID02) looks right I will push the GC2145 on top of this branch then.

avolmat-st · 2025-05-13T17:52:59Z

I saw in the code that for the menu type it says that this is for standard or driver defined control. However I have a doubt about that since in case of ctrl which are not standard, the set_type_flag function is going to fallback to INTEGER type ctrl, hence not being possible to define custom menu ctrl I think. In order to achieve this it would actually be necessary to add a new parameter to the init functions if it is not a known ctrl.

This is not really related to this PR itself in fact.

ngphibang

The integer menu controls LGTM. Thanks !

drivers/video/video_ctrls.h

drivers/video/video_ctrls.c

ngphibang · 2025-05-13T19:54:55Z

I saw in the code that for the menu type it says that this is for standard or driver defined control. However I have a doubt about that since in case of ctrl which are not standard, the set_type_flag function is going to fallback to INTEGER type ctrl, hence not being possible to define custom menu ctrl I think. In order to achieve this it would actually be necessary to add a new parameter to the init functions if it is not a known ctrl.

"Driver-defined menu" means the driver defines the menu items but the control id is known, it does not mean a private & driver-defined items menu control (I didn't see such a control, in Linux). Two examples are the test_pattern (driver-defined item) and the power line frequency (standard):

	ret = video_init_menu_ctrl(&ctrls->light_freq, dev, VIDEO_CID_POWER_LINE_FREQUENCY,
				   VIDEO_CID_POWER_LINE_FREQUENCY_50HZ, NULL);

	ret = video_init_menu_ctrl(&ctrls->test_pattern, dev, VIDEO_CID_TEST_PATTERN, 0,
				   test_pattern_menu);

avolmat-st · 2025-05-13T19:58:16Z

I saw in the code that for the menu type it says that this is for standard or driver defined control. However I have a doubt about that since in case of ctrl which are not standard, the set_type_flag function is going to fallback to INTEGER type ctrl, hence not being possible to define custom menu ctrl I think. In order to achieve this it would actually be necessary to add a new parameter to the init functions if it is not a known ctrl.

"Driver-defined menu" means the driver defines the menu items but the control id is known, it does not mean a private & driver-defined menu control (I didn't see such a control, in Linux). Two examples are the test_pattern (driver-defined item) and the power line frequency (standard):
	ret = video_init_menu_ctrl(&ctrls->light_freq, dev, VIDEO_CID_POWER_LINE_FREQUENCY,
				   VIDEO_CID_POWER_LINE_FREQUENCY_50HZ, NULL);

	ret = video_init_menu_ctrl(&ctrls->test_pattern, dev, VIDEO_CID_TEST_PATTERN, 0,
				   test_pattern_menu);

Understood. Right I kind of read it as driver defined control ;) OK then. Thanks for the clarification.

ngphibang

LGTM, just wait for the dependency (CCI PR) get merged first

avolmat-st · 2025-05-17T20:12:35Z

@ngphibang I am having kind of second thought about the link frequency helper. Should we really have that or should we simply enforce that any CSI driver MUST expose a LINK_FREQUENCY control. Currently, with the addition of the GC2145 csi, I think that is only 2 sensors being able to work in CSI. I will add the LINK_FREQUENCY ctrl to the GC2145 so only the OV5640 won't have. I guess we could add it. If we decide to enforce the requirement of LINK_FREQUENCY ctrl that helper doesn't really make sense. What do you think ?

ngphibang · 2025-05-18T06:00:31Z

@ngphibang I am having kind of second thought about the link frequency helper. Should we really have that or should we simply enforce that any CSI driver MUST expose a LINK_FREQUENCY control. Currently, with the addition of the GC2145 csi, I think that is only 2 sensors being able to work in CSI. I will add the LINK_FREQUENCY ctrl to the GC2145 so only the OV5640 won't have. I guess we could add it. If we decide to enforce the requirement of LINK_FREQUENCY ctrl that helper doesn't really make sense. What do you think ?

I think both are relevant and describe different aspects of things. Pixel rate is simpler to be used. Personally, I would not enforce anything if it's technically not wrong (e.g. if someone finds that link frequency is better they can make change btw).

avolmat-st · 2025-05-18T06:28:20Z

@ngphibang I am having kind of second thought about the link frequency helper. Should we really have that or should we simply enforce that any CSI driver MUST expose a LINK_FREQUENCY control. Currently, with the addition of the GC2145 csi, I think that is only 2 sensors being able to work in CSI. I will add the LINK_FREQUENCY ctrl to the GC2145 so only the OV5640 won't have. I guess we could add it. If we decide to enforce the requirement of LINK_FREQUENCY ctrl that helper doesn't really make sense. What do you think ?

I think both are relevant and describe different aspects of things. Pixel rate is simpler to be used. Personally, I would not enforce anything if it's technically not wrong (e.g. if someone finds that link frequency is better they can make change btw).

To me the real important value in case of CSI is the LINK FREQUENCY since this is that link frequency which is related to the UI necessary for configuration of a CSI PHY. From the pixel rate we can only kind of guess a link frequency "enveloppe" that would be necessary to transfer THAT amount of pixel but that's not necessarily true and the exact value. How it is transmitted and the amount of pixels are 2 different aspects I think.
However as you said, if this turns out not to be used or turns out it should be there later on we can remove it ;) So let's not spend too much time on this now.

@loicpoulain the MIPI CSI2 DT header is not part of this serie instead of the GC2145 serie (since it is also necessary for the ST-MIPI which is a dependency for the GC2145 since it introduce the LINK_FREQUENCY ctrl).

josuah · 2025-05-18T10:15:54Z

I opened this issue to help keeping track of everywhere link frequency (and other related API changes) are discussed:

drivers: video: link frequencies, blanking, flip-aware formats #90099

avolmat-st · 2025-05-24T20:10:28Z

Pushed updates based on recent review points (still open points related to adjustments related to Linux).
Diff shows changes within the CCI functions, but that's simply because I use the latest / merged commit instead. This is actually removed the next force push since I rebased on the head which already contains this commit

Next I push the same commits rebased on top of head in order to allow proper build.

Add standard data-type macros within the video.h in order to avoid having to redefine them all in each and every CSI based driver. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

avolmat-st · 2025-05-26T19:41:01Z

Actually this PR and the GC2145 PR (89571) both depend on each other.
The MIPID02 needs the GC2145 update in order to properly put the test case and the GC2145 PR needs the LINK_FREQ and MIPI_DT type to be defined.

Turns out I will have to move back the LINK_FREQ defintion and MIPI_DT type defintion back into the GC2145 PR in order to be able to first merge the GC2145/CSI PR.

Maybe I will first wait to have all discussion closed in this thread prior to moving the commits to the GC2145/CSI PR. Sorry for the mess ...

josuah

One might need some attention (possibly a bug?).

Everything else is more about feedback long-term, and less important details IMHO.

Very interesting part! Hope this gets some public doc soon so we can use it outside ST's DKs. :)

drivers/video/video_ctrls.h

drivers/video/video_common.c

drivers/video/video_st_mipid02.c

avolmat-st · 2025-05-29T09:58:47Z

@josuah , updated based on your remarks. This is actually linked to the GC2145 / CSI PR, (tests can't be run without having GC2145 PR merged as well and GC2145 PR needs the LINK_FREQ CID which is in THIS PR), should I actually merge both PR here ? I could simply add the 3 GC2145 commits which enable CSI within this PR which would make the merge easier ?

The other approach is that I add only the relevant commits from this PR into of the GC2145 PR so that GC2145 PR can be merged first.

josuah · 2025-05-30T22:27:22Z

GC2145 PR needs the LINK_FREQ CID which is in THIS PR), should I actually merge both PR here ?
I could simply add the 3 GC2145 commits which enable CSI within this PR which would make the merge easier ?

Putting the hardware-independent commits (CID, MIPI data types) might work. This should be quick as most of it is reviewed here already and these additions can also be useful in downstream applications.

The other approach is that I add only the relevant commits from this PR into of the GC2145 PR so that GC2145 PR can be merged first.

If this is easy to move the commits around, then this works too! Whichever is most straightforward.

avolmat-st · 2025-05-31T14:38:18Z

Updated test part, this PR can thus be merged without dependencies.

ngphibang · 2025-06-01T18:37:54Z

include/zephyr/drivers/video-controls.h

@@ -362,6 +362,9 @@ enum video_camera_orientation {
 */
 #define VIDEO_CID_IMAGE_PROC_CLASS_BASE 0x009f0900

+/** Link frequency, applicable for the CSI2 based devices. This control is read-only. */


Since it is not read-only anymore, should we update this and the commit message too ?

Indeed, I correct this and push it. Since those commits are also part of the GC2145/CSI PR (in order to be able to have CI OK, I also update on the GC2145/CSI PR as well).

Add a new INTEGER_MENU type allowing to store signed 64bits integer into a menu. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

Add a ctrl VIDEO_CID_LINK_FREQ to indicate to a sink the frequency at which a device streams data over CSI2. Since not all source device currently provide the LINK_FREQ control, add a helper function to retrieve it, with a fall-back by doing an approximate via the PIXEL_RATE control. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

Addition of the support for the CSI to DVP bridge ST MIPID02. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

ngphibang

LGTM, thanks !

sonarqubecloud · 2025-06-01T19:36:13Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions bot added the area: Video Video subsystem label May 10, 2025

github-actions bot requested review from josuah, loicpoulain and ngphibang May 10, 2025 14:59

avolmat-st force-pushed the video_st_mipid02_driver branch from f9c37d0 to 0b3ce16 Compare May 10, 2025 15:51

josuah previously approved these changes May 10, 2025

View reviewed changes

avolmat-st mentioned this pull request May 11, 2025

clock_control: stm32: mp13: add plls / mco & clock source selection #89773

Merged

avolmat-st dismissed josuah’s stale review via 8dd7671 May 11, 2025 16:51

avolmat-st force-pushed the video_st_mipid02_driver branch from 0b3ce16 to 8dd7671 Compare May 11, 2025 16:51

avolmat-st force-pushed the video_st_mipid02_driver branch from 8dd7671 to 5489363 Compare May 12, 2025 13:52

avolmat-st mentioned this pull request May 12, 2025

video: gc2145: addition of CSI output support #89571

Merged

avolmat-st force-pushed the video_st_mipid02_driver branch from 5489363 to 6406a5a Compare May 12, 2025 20:14

ngphibang reviewed May 13, 2025

View reviewed changes

avolmat-st force-pushed the video_st_mipid02_driver branch from 6406a5a to 461001a Compare May 13, 2025 17:43

avolmat-st force-pushed the video_st_mipid02_driver branch from 461001a to dbd1db7 Compare May 13, 2025 18:44

ngphibang reviewed May 13, 2025

View reviewed changes

drivers/video/video_ctrls.h Outdated Show resolved Hide resolved

drivers/video/video_ctrls.c Outdated Show resolved Hide resolved

avolmat-st force-pushed the video_st_mipid02_driver branch from dbd1db7 to 34f00e9 Compare May 13, 2025 20:25

ngphibang reviewed May 16, 2025

View reviewed changes

josuah mentioned this pull request May 18, 2025

drivers: video: link frequencies, blanking, flip-aware formats #90099

Open

josuah mentioned this pull request May 20, 2025

drivers: video: common: introduce CCI utilities #87935

Merged

erwango assigned josuah May 21, 2025

avolmat-st force-pushed the video_st_mipid02_driver branch from a3108e8 to 39f7f23 Compare May 24, 2025 20:08

video: addition of CSI2 standard data-type defines

f372b70

Add standard data-type macros within the video.h in order to avoid having to redefine them all in each and every CSI based driver. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

avolmat-st force-pushed the video_st_mipid02_driver branch 2 times, most recently from 4864ad7 to 784baf7 Compare May 24, 2025 20:18

josuah self-requested a review May 26, 2025 00:52

avolmat-st force-pushed the video_st_mipid02_driver branch from 784baf7 to 45f87ef Compare May 26, 2025 18:56

josuah requested changes May 29, 2025

View reviewed changes

avolmat-st force-pushed the video_st_mipid02_driver branch from 45f87ef to e408b75 Compare May 29, 2025 09:55

avolmat-st force-pushed the video_st_mipid02_driver branch 3 times, most recently from f98b313 to b6d62ff Compare May 31, 2025 13:56

josuah requested a review from ngphibang May 31, 2025 23:05

josuah previously approved these changes May 31, 2025

View reviewed changes

ngphibang reviewed Jun 1, 2025

View reviewed changes

Alain Volmat added 3 commits June 1, 2025 21:02

video: common: addition of INTEGER_MENU control type

7cf6f2f

Add a new INTEGER_MENU type allowing to store signed 64bits integer into a menu. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

video: st_mipid02: addition of ST MIPID02 CSI bridge

b29e417

Addition of the support for the CSI to DVP bridge ST MIPID02. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

avolmat-st dismissed josuah’s stale review via b29e417 June 1, 2025 19:05

avolmat-st force-pushed the video_st_mipid02_driver branch from b6d62ff to b29e417 Compare June 1, 2025 19:05

ngphibang approved these changes Jun 1, 2025

View reviewed changes

josuah approved these changes Jun 1, 2025

View reviewed changes

kartben merged commit eeab052 into zephyrproject-rtos:main Jun 2, 2025
26 checks passed

video: st_mipid02: addition of ST MIPID02 CSI bridge #89764

video: st_mipid02: addition of ST MIPID02 CSI bridge #89764

Uh oh!

Conversation

avolmat-st commented May 10, 2025

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

josuah May 10, 2025

Choose a reason for hiding this comment

Uh oh!

avolmat-st May 11, 2025

Choose a reason for hiding this comment

Uh oh!

josuah May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avolmat-st May 11, 2025

Choose a reason for hiding this comment

Uh oh!

ngphibang May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avolmat-st commented May 12, 2025

Uh oh!

avolmat-st commented May 12, 2025

Uh oh!

avolmat-st commented May 12, 2025

Uh oh!

ngphibang May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avolmat-st May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ngphibang May 13, 2025

Choose a reason for hiding this comment

Uh oh!

avolmat-st commented May 13, 2025

Uh oh!

avolmat-st commented May 13, 2025

Uh oh!

ngphibang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ngphibang commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avolmat-st commented May 13, 2025

Uh oh!

ngphibang left a comment

Choose a reason for hiding this comment

Uh oh!

avolmat-st commented May 17, 2025

Uh oh!

ngphibang commented May 18, 2025

Uh oh!

avolmat-st commented May 18, 2025

Uh oh!

josuah commented May 18, 2025

Uh oh!

avolmat-st commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avolmat-st commented May 26, 2025

Uh oh!

josuah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josuah May 11, 2025 •

edited

Loading

ngphibang May 11, 2025 •

edited

Loading

ngphibang May 13, 2025 •

edited

Loading

ngphibang commented May 13, 2025 •

edited

Loading

avolmat-st commented May 24, 2025 •

edited

Loading

josuah left a comment •

edited

Loading